-
Notifications
You must be signed in to change notification settings - Fork 39
refactor(tests): split up test suites, update docs #278
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
refactor(tests): split up test suites, update docs #278
Conversation
c0a4cb6
to
0c43f12
Compare
0c43f12
to
6d76607
Compare
OK, so this PR is ready for review, but I'm a little dissatisfied with the consistency of CI -- running with more concurrency seems to have made tests more unstable. Going to re-run the latest job a few more times to make sure it passes most of the time so that the average PR should pass without issues. |
OK unfortunately there were still a couple failures -- (mac/windows + weval). I wonder if we can get away with disabling test concurrency for weval runs... If that's more consistent then I'll go with that, otherwise I'll just disable test-level concurrency all together (suite level concurrency still happens) |
OK, going to rip out concurrency here -- tests consistently passing is more important I think. [EDIT] - Actually the test that is still failing is a Weval one which is timing out... Updating test config to run only one concurrent suite at a time if we're doing that. |
@vados-cosmonic same request I often have: can you update the description here to add some motivation for this? It's so very much easier to review PRs when I know which lens to look through. It's also very useful for looking at changes later on. |
Hey @tschneidereit yeah it's been a while on this PR -- I believe I was trying to finish the switch to In particular this comment here: When not in CI (on really slow machines), this PR makes the tests finish faster by separating one large suite into multiple (that can run in parallel under |
Yeah, my apologies for not getting to the review sooner. If anything though, this drives home why explanations for changes are so important: they're quite often hard to derive from the changes themselves after the fact. Regarding the reasoning you mention, if I read this comment correctly, that doesn't really apply anymore? If that's correct, do we even want this change still? And if yes, please do update the PR description with an explanation of what the PR does, and why. |
Ah so the tests still run faster, they just don't run as fast as they should have in CI due to the weval failures when run concurrently, they still run much faster locally. Note that all the non-weval builds still finish faster. Will update the PR description! |
When not in CI (on really slow machines), this PR makes the tests finish faster by separating one large suite into multiple (that can run in parallel under vitest).
Tests also run faster locally with these changes, as all the tests can run in parallel and even weval tests will pass normally on a modern machine.